-
Notifications
You must be signed in to change notification settings - Fork 80
Add start-runner, restart-runner and stop-runner commands #216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR refactors the install and uninstall runner commands to share common logic via new runnerOptions and cleanupOptions abstractions, introduces three new CLI commands (start-runner, stop-runner, restart-runner) that leverage the shared logic, and updates documentation to surface and describe these new commands and minor comment corrections. Sequence diagram for restart-runner command executionsequenceDiagram
actor User
participant CLI
participant Docker
User->>CLI: run restart-runner
CLI->>Docker: stop runner container (no image/model removal)
Docker-->>CLI: container stopped
CLI->>Docker: start runner container (no image pull)
Docker-->>CLI: container started
Class diagram for runnerOptions and cleanupOptions abstractionsclassDiagram
class runnerOptions {
+uint16 port
+string gpuMode
+bool doNotTrack
+bool pullImage
}
class cleanupOptions {
+bool models
+bool removeImages
}
class install_runner_command {
+RunE(cmd, args)
}
class start_runner_command {
+RunE(cmd, args)
}
class restart_runner_command {
+RunE(cmd, args)
}
class stop_runner_command {
+RunE(cmd, args)
}
class uninstall_runner_command {
+RunE(cmd, args)
}
install_runner_command --> runnerOptions
start_runner_command --> runnerOptions
restart_runner_command --> runnerOptions
restart_runner_command --> cleanupOptions
stop_runner_command --> cleanupOptions
uninstall_runner_command --> cleanupOptions
class runInstallOrStart {
+runInstallOrStart(cmd, runnerOptions)
}
class runUninstallOrStop {
+runUninstallOrStop(cmd, cleanupOptions)
}
install_runner_command --> runInstallOrStart
start_runner_command --> runInstallOrStart
restart_runner_command --> runInstallOrStart
restart_runner_command --> runUninstallOrStop
stop_runner_command --> runUninstallOrStop
uninstall_runner_command --> runUninstallOrStop
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @ericcurtin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Docker Model Runner CLI by adding dedicated commands to start and stop the model runner container. These new commands provide users with direct control over the container's state, particularly in standalone environments where Docker Desktop or a manually configured host is not in use. The changes include the command definitions, the underlying functions for container interaction, and updated documentation to reflect the new functionality. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds Docker CLI commands to start and stop the model runner container, providing basic lifecycle management functionality.
- Implements
StartControllerContainerandStopControllerContainerfunctions with container lookup and state management - Creates new CLI commands
docker model startanddocker model stopwith proper error handling for unsupported contexts - Adds comprehensive documentation files for both commands
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/cli/pkg/standalone/containers.go | Adds container start/stop functions with proper error handling and status reporting |
| cmd/cli/commands/start-runner.go | Implements CLI command for starting model runner containers |
| cmd/cli/commands/stop-runner.go | Implements CLI command for stopping model runner containers |
| cmd/cli/commands/root.go | Registers new start and stop commands with the CLI |
| cmd/cli/docs/reference/model_start.md | Documentation for the start command |
| cmd/cli/docs/reference/model_stop.md | Documentation for the stop command |
| cmd/cli/docs/reference/docker_model_start.yaml | YAML metadata for start command documentation |
| cmd/cli/docs/reference/docker_model_stop.yaml | YAML metadata for stop command documentation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces docker model start and docker model stop commands to manage the lifecycle of the Docker Model Runner container. The implementation adds new cobra commands, corresponding business logic in the standalone package, and documentation.
My review has identified a critical issue in the start command's implementation that will prevent it from working as intended. There's also a high-severity issue with the stop command regarding idempotency and side effects. Additionally, I've provided some medium-severity suggestions to improve code quality and maintainability in the new command files and the container management logic.
doringeman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it supposed to work to docker model stop + docker model start and have the same container back up?
|
@doringeman start/stop is supposed to be like install or uninstall without: docker pull and docker rmi -f if that makes sense, this is incomplete though. |
f01d574 to
4444765
Compare
4444765 to
31693c3
Compare
31693c3 to
c9119e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
c9119e3 to
047cad5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The repeated flag definitions for --port, --gpu, and --do-not-track across install, start, and restart commands could be DRYed by extracting a shared helper that registers runner flags and binds them to runnerOptions.
- Consider renaming runInstallOrStart and runUninstallOrStop to more descriptive names (e.g. startOrInstallRunner, stopOrUninstallRunner) to improve readability and intent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated flag definitions for --port, --gpu, and --do-not-track across install, start, and restart commands could be DRYed by extracting a shared helper that registers runner flags and binds them to runnerOptions.
- Consider renaming runInstallOrStart and runUninstallOrStop to more descriptive names (e.g. startOrInstallRunner, stopOrUninstallRunner) to improve readability and intent.
## Individual Comments
### Comment 1
<location> `cmd/cli/docs/reference/model_restart-runner.md:4` </location>
<code_context>
+# docker model restart-runner
+
+<!---MARKER_GEN_START-->
+Start Docker Model Runner (Docker Engine only)
+
+### Options
</code_context>
<issue_to_address>
**suggestion:** Heading should say 'Restart Docker Model Runner' instead of 'Start'.
Update the heading to 'Restart Docker Model Runner (Docker Engine only)' to better reflect the command's purpose and avoid confusion.
```suggestion
Restart Docker Model Runner (Docker Engine only)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
047cad5 to
c8870fb
Compare
c8870fb to
28b9194
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
28b9194 to
0e8148e
Compare
58d5fce to
729a345
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
9519b23 to
9cfaf33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
9cfaf33 to
8a440ed
Compare
8a440ed to
84f9e30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
84f9e30 to
1eb57a0
Compare
1eb57a0 to
fcf7653
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cmd/cli/commands/stop-runner.go
Outdated
| var models bool | ||
| c := &cobra.Command{ | ||
| Use: "stop-runner", | ||
| Short: "Stop Docker Model Runner", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Short: "Stop Docker Model Runner", | |
| Short: "Stop Docker Model Runner (Docker Engine only)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sprinkling more of these around thanks
| model-distribution-tool | ||
| model-runner | ||
| model-runner.sock | ||
| docker-model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is docker-model?
To start, restart and stop container without pulling or removing the image. Signed-off-by: Eric Curtin <[email protected]>
fcf7653 to
edcc9e5
Compare
To start, restart and stop container without pulling or removing the image.